Skip to content

Conversation

Zeophlite
Copy link
Contributor

@Zeophlite Zeophlite commented Aug 26, 2025

Objective

Testing

  • CI

ickshonpe and others added 30 commits July 30, 2025 18:25
* `TextInputAction` -> `TextEdit`
* `TextInputActions` -> `TextEdits`
…he cursor. If there is a selection, overwrites it instead.
…d automatically with the `TextInputValue `text
…ts to `InvalidEdit` and `TextChanged` respectively.
@Zeophlite
Copy link
Contributor Author

Hey @tigregalis , I think the below addresses your feedback for this release, and the rest can be deferred for another PR?

I don't understand space_advance - I assume it's working around some bug or limitation: what's the issue it's solving?

I'm not super sure either, @ickshonpe @viridia can you chime in?

Text is always joined via a "\n" but for correctness you might use the BufferLine's LineEnding (e.g. on windows it might be \r\n if coming from a file or a paste), but I'm not 100% sure on this.

I'm not sure on this, we just call editor.action(Action::Enter); and cosmic_text handles that

20.0 is used as a magic constant in a few places, can we make it a named constant(s)?

Done

As a default, line height of 1.2x font size is used generally across web, and looks good generally. For example I think it's in firefox's user agent stylesheet if I'm not wrong.

Added a comment to reflect this

Comment on Paste should note it replaces the current selection, not inserting at the cursor. More broadly, all insertions replace the current selection, sometimes the selection just has a width of 0.

Added a comment

TextEdit::Submit feels like it belongs in the realm of TextInput being a full widget, maybe even as part of a Form, so its inclusion is odd when there's not yet a full widget. It's the only variant that is special-cased in the apply_text_edits handler system. It's also special-cased in the apply_text_edit function. I would be inclined to remove it and revisit it as a higher level action with the text input widget, and not tied at all to text editing.

I agree, I've separated the "submission" into the example, this could be merged back later, depending on how "forms" look

@Zeophlite Zeophlite added S-Needs-Review Needs reviewer attention (from anyone!) to move forward and removed S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged labels Aug 30, 2025
@viridia
Copy link
Contributor

viridia commented Aug 30, 2025

I'm not sure what space_advance is. However, what I can tell you is that this is an area in which desktop UIs vary.

Early terminal displays did not support I-beam style cursors, and could only blink a fixed-width character on and off; when the cursor was at the end of the line, it would blink an empty space.

In the original MacOS UI guidelines, and for many years after, the standard behavior for most GUIs was that when you selected the invisible line-break character at the end of a wrapped line, the selection highlight would be extended all the way to the right of the text input box, that is, the left edge of the rectangle would start just after the last character on the line, and the right edge of the rectangle would be the inner right padding of the containing box.

However, some modern apps (like Chrome and VSCode) do not do this: instead, selecting the wrap character produces a cursor of some fixed width. When there is no wrap character (that is, either the cursor is at the end of the text, or it broke the line in the middle of a word) it shows a normal I-beam style cursor.

@tigregalis
Copy link
Contributor

Hey @tigregalis , I think the below addresses your feedback for this release, and the rest can be deferred for another PR?

Yeah, sounds good.

I am a little hesitant about the password mask and the input filter being part of the same PR, but if those are the features people are looking for ultimately, maybe it doesn't make sense to hold them back for the sake of code history/evolution clarity.

Copy link
Contributor

@atlv24 atlv24 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i am concerned about how much code is in the example. additionally, it seems this is just for 2d, can it be used in a ui in 3d or etc easily?

@Zeophlite
Copy link
Contributor Author

i am concerned about how much code is in the example. additionally, it seems this is just for 2d, can it be used in a ui in 3d or etc easily?

There's a lot in the example as this is the "low level" api - the follow-up PR has the "mid level" api for widgets, etc. Have a look at the examples/ in https://github.com/bevyengine/bevy/pull/20326/files .

Copy link
Member

@mockersf mockersf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've played a bit with this PR and how to use it, and I think it introduces too many things at once, and some that doesn't interact well together.

I would prefer a simpler PR that would introduce a component that holds a cosmic buffer, and how to interact with it. Making it smaller would also improve the code organisation.

I don't think this should be merged in this state.

@Zeophlite
Copy link
Contributor Author

and some that doesn't interact well together

@mockersf Could you expand on this point?

@alice-i-cecile alice-i-cecile added S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Sep 1, 2025
@alice-i-cecile alice-i-cecile modified the milestones: 0.17, 0.18 Sep 1, 2025
@Zeophlite
Copy link
Contributor Author

I've taken the time to split up input.rs

@Zeophlite
Copy link
Contributor Author

Zeophlite commented Sep 2, 2025

I have also taken the time to adopt #20326 , rebase onto this branch - see #20826

This was very helpful in fleshing out some concepts, such as "submit"

I appreciate any feedback on either repo on what parts should be pulled out to help merge these

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Text Rendering and layout for characters A-UI Graphical user interfaces, styles, layouts, and widgets C-Feature A new feature, making something new possible M-Needs-Release-Note Work that should be called out in the blog due to impact S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged X-Contentious There are nontrivial implications that should be thought through
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants